-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(wakunode2): enable libp2p rendezvous protocol by default #1770
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a little comment
tests/v2/test_waku_switch.nim
Outdated
|
||
await allFutures(wakuSwitch.start(), sourceSwitch.start(), destSwitch.start()) | ||
|
||
# Connect clients to the rendezvous pint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typo
# Connect clients to the rendezvous pint | |
# Connect clients to the rendezvous point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! All my comments below are nitpicks (except perhaps moving the test to a different suite), so feel free to address/ignore as you see fit.
tests/v2/test_waku_switch.nim
Outdated
@@ -100,3 +111,37 @@ suite "Waku Switch": | |||
|
|||
## Teardown | |||
await allFutures(wakuSwitch.stop(), sourceSwitch.stop(), destSwitch.stop()) | |||
|
|||
asyncTest "Waku Switch uses Rendezvous": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't include this in the switch
tests. Since this is a separate protocol I suggest creating a new test_waku_rendezvous
suite (even if it contains only the one test for now).
waku/v2/node/waku_switch.nim
Outdated
@@ -9,7 +9,7 @@ import | |||
chronos, chronicles, | |||
eth/keys, | |||
libp2p/crypto/crypto, | |||
libp2p/protocols/pubsub/gossipsub, | |||
libp2p/protocols/[pubsub/gossipsub, rendezvous], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally have imports fully qualified and in separate lines (collective imports like this are difficult to "find and replace" at later stage). See coding guidelines
waku/v2/node/waku_node.nim
Outdated
@@ -21,6 +21,7 @@ import | |||
libp2p/protocols/connectivity/autonat/service, | |||
libp2p/nameresolving/nameresolver, | |||
libp2p/builders, | |||
libp2p/protocols/rendezvous, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very much a nitpick: while builders
above clearly doesn't follow this practice, we tend to sort imports more or less alphabetically (with the exception of common imports (std
chronos
metrics
etc.) which are usually top-level). Not a strict rule, but would be great for this import to fit in with the other libp2p/protocols
above :)
Will be good to do interop test with go-waku next to see if it works. Does the underlying implementation contain any metrics which we can add to our monitoring dashboards? |
Fixed all the issues in comments. Will reach out to go-waku for the interop test Sadly, no metrics are exposed in the libp2p implementation, might be worth openning a PR on nim-libp2p to add some? |
Can confirm that go-waku can use nwaku as a rendezvous point in this PR! 🚀 |
Description
Rendezvous Discovery protocol allows peers to advertise their addresses at a rendezvous point and other peers to query the rendezvous point to discover new peers.
This will be particularly useful for peers using circuit relay as they can then easily advertise their circuit relay multiaddress and be discovered by other network participants.
This PR uses libp2p Rendezvous implementation and mounts the protocol by default when relay protocol is enabled.
Changes
Issue
closes #1605